Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt FFI #10221

Closed
wants to merge 57 commits into from
Closed

Adopt FFI #10221

wants to merge 57 commits into from

Conversation

JasonLunn
Copy link
Contributor

@JasonLunn JasonLunn commented Jul 6, 2022

Opening this PR in order to get visibility on any potential Kokoro issues introduced by the branch.

# Conflicts:
#	src/google/protobuf/compiler/java/message_serialization.cc
…ementation of FFI.

Code cleanup: fix indentation and some other sources of spurious warnings.
…mbol, unlike rb_define_const, so skip its invocation after warning.
…etween Ruby and JRuby.

Preserves distinct conformance test expectations, even though Ruby and JRuby are currently identical.
@JasonLunn JasonLunn added ruby kokoro:run jruby Issues unique to the JRuby interpreter labels Jul 6, 2022
Includes minor build file fixes and tweaks to avoid warnings when running tests.
@JasonLunn
Copy link
Contributor Author

This PR passes all tests again.

@mkruskal-google for review of changes to Bazel-ization.
@haberman - there are a few TODO messages left in this PR that I'd like to review with you at your convenience.

@JasonLunn JasonLunn marked this pull request as ready for review August 28, 2022 20:35
@JasonLunn JasonLunn changed the title [WIP] Adopt FFI Adopt FFI Aug 28, 2022
@haberman
Copy link
Member

It looks like this PR is proposing to migrate all of JRuby and CRuby to FFI.

We cannot migrate CRuby until we have validated the performance. Do you have performance numbers?

Last we talked my understanding was that this regresses CRuby significantly.

@JasonLunn
Copy link
Contributor Author

It turns out that all performance was significantly negatively impacted by the same issue that was causing memory corruption. Now that has been addressed, I think there is a more compelling case for a general migration. I can post the numbers here or share them with you when I see you later this week.

.bazelrc Outdated Show resolved Hide resolved
@haberman
Copy link
Member

Closing this PR in favor of #11483

@haberman haberman closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jruby Issues unique to the JRuby interpreter ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants